Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle INT signal correctly #646

Merged
merged 1 commit into from
Apr 3, 2024
Merged

Handle INT signal correctly #646

merged 1 commit into from
Apr 3, 2024

Conversation

tompng
Copy link
Member

@tompng tompng commented Mar 19, 2024

Handle SIGINT and SIGWINCH correctly, (just update ivar)
inner_getc will periodically check if these signal is fired.
Changed signal check timeout from 0.1 to 0.01. C-c is handled here, so 0.1s is too slow. (C-c will delay 0.1s)

Remove handle_cleared because clear(C-l) is not a signal

This will fix #461
This will fix C-c problem when binding.irb is executed in non-main thread. ruby -e "Thread.new{binding.irb}.join"

Another possible way to correctly handle SIGINT is to use input.raw{getc == "\C-c"} instead of using Signal.trap and input.raw(intr:true){}.
But we have to handle other signals too. (example: \C-z). We also have to think for windows.

Note: This pull request DOES NOT fix multiple readline in multithread problem 2.times{Thread.new{Reline.readline'>'}}

@tompng tompng force-pushed the sigint branch 3 times, most recently from 276e1ca to 70cba12 Compare March 25, 2024 11:43
@ima1zumi ima1zumi added the bug Something isn't working label Mar 25, 2024
@@ -278,7 +284,7 @@ def self.cursor_pos
buf = @@output.pread(@@output.pos, 0)
row = buf.count("\n")
column = buf.rindex("\n") ? (buf.size - buf.rindex("\n")) - 1 : 0
rescue Errno::ESPIPE
rescue Errno::ESPIPE, IOError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my learning: Why did you need IOError?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is because pread raises IOError in some situation #537.
Removed this change because it is out of scope of this pull request.

lib/reline.rb Show resolved Hide resolved
Reline::HISTORY << whole_buffer
end
whole_buffer = line_editor.whole_buffer.dup
whole_buffer.taint if RUBY_VERSION < '2.7'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the taint call necessary for Ruby 2.6? If not, I think it's fine to remove it now as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kernel#gets and other string read from IO are tainted in Ruby 2.6 https://docs.ruby-lang.org/en/2.6.0/Object.html#method-i-taint
Reline should return tainted string to align with Kernel.gets.
I'm not sure of the impact of removing taint. Perhaps nobody is using tainted?, but I'd like to keep this until dropping 2.6 support.

# Code relying on taint security model
Foo::Settings.map do |s|
  if s.tainted?
    s=~/\A\d+\z/ ? s.to_i : s
  else
    eval s
  end
end

@@ -1089,6 +1089,7 @@ def input_key(key)
end
end
if key.char.nil?
process_insert(force: true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is process_insert needed in key.char.nil? == true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's needed but not related to this pull request. I'll open another one with test code

Copy link
Member

@ima1zumi ima1zumi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is failing, but I think it isn't because of the code change. LGTM!

@tompng tompng merged commit 3debb0a into ruby:master Apr 3, 2024
39 of 40 checks passed
@tompng tompng deleted the sigint branch April 3, 2024 15:50
matzbot pushed a commit to ruby/ruby that referenced this pull request Apr 4, 2024
artur-intech pushed a commit to artur-intech/ruby that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

Thread deadlock in multiline command editing
3 participants